-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add article, article and client property value, article and client tag and some documentation. #20
base: master
Are you sure you want to change the base?
Conversation
…pdated other documentation.
# @return [Billomat::Actions::Complete] | ||
# | ||
# @example | ||
# Billomat::Actions::Complete('12345', { template_id: '10231' }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take care of the copy paste glitches. Billomat::Actions::Complete
all over the place, correct to Billomat::Actions::Correct
.
end | ||
|
||
# @param [Hash] paging_data The response from get_paging_data | ||
def self.out_of_bounds(paging_data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conventionally this helper should be named out_of_bound?
as it returns a boolean.
# @param [Hash] paging_data The response from get_paging_data
# @return [Boolean] whenever the given paging data is out of bound or not
def self.out_of_bound?(paging_data)
# @param [Hash] resp The response from the gateway | ||
# @param [String] name The name of the resource | ||
# @return [Hash] The paging info (page, per_page and total) | ||
def self.get_paging_data(resp, name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conventionally Ruby methods do not prefix get_
or set_
- just name it paging_data
.
module Billomat | ||
# This class provides the possibility utility functions for data formatting | ||
class Utils | ||
def self.get_sufix(name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method/design is not robust. Unfortunately we do not require/added ActiveSupport as runtime dependency - so we do not have String#pluralize
, but it would be better to return the fully processed input string instead of returning a string suffix.
For future ActiveSupport addition it would be great if you could refactor this like so:
def self.pluralized_resource(name)
# NOTE: When ActiveSupport gets included convert this to +String.pluralize+.
return 'taxes' if name == 'tax'
"#{name}s"
end
Then call it like this later:
resp[pluralized_resource(name)]['@total'].to_i
It is also not required prefix Billomat::Utils
here, inside the same file to use a static method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there's already resource_name
, why not handle it within the model(s), e.g.:
module Billomat
module Models
class Tax < Base
def self.base_path
'/taxes'
end
def self.base_name
'taxes'
end
def self.resource_name
'tax'
end
end
end
end
base_path
is so generic that it can probably be moved into Billomat::Models::Base
:
def self.base_path
"/#{base_name}"
end
This way, you'd have:
resp[@resource.base_name]['@total'].to_i
# or
resp[@resource.base_name][@resource.resource_name]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with this, too!
lib/billomat/models/base.rb
Outdated
oob = Billomat::Utils.out_of_bounds(paging_data) | ||
data = oob ? [] : Billomat::Utils.to_array(resp, resource_name, self) | ||
|
||
{ 'paging_data' => paging_data, 'data' => data } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A mapped object with method access would be nicer to read/use at the clients.
article_tags = Billomat::Models::ArticleTag.paged_list(page = 2, per_page = 10)
pp article_tags.data
# => [Array<Billomat::Models::ArticleTag>]
pp article_tags.page
# => Int
pp article_tags.per_page
# => Int
pp article_tags.total
# => Int
Thank you very much for your effort! This PR is a great enhancement for the project, nice! 🎖️ I just left some minor comments and refactorings, but nothing to fancy. Please add new line(s) describing the changes (all suffixed with Another good thing would be some specs - at least for the new models like: https://github.com/hausgold/billomat/blob/master/spec/models/invoice_spec.rb |
Hi Jack, I'll look into the suggestions. |
fix typo Co-authored-by: Hermann Mayer <hermann.mayer92@gmail.com>
Co-authored-by: Hermann Mayer <hermann.mayer92@gmail.com>
Co-authored-by: Hermann Mayer <hermann.mayer92@gmail.com>
Co-authored-by: Hermann Mayer <hermann.mayer92@gmail.com>
Co-authored-by: Hermann Mayer <hermann.mayer92@gmail.com>
I’ve added some code to support the way we plan to use Billomat and added some documentation for my coworkers. I’m interested in getting my current (and future) work mainlined. My changes are:
added list and paed list methods to base model
added utils and moved to_array and count to it
added models:
article
article property value
article tag
client property value
client tag
added documentation for:
articles
clients
invoices